Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement stats calculations for new calculated_stats_ container #468

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

TinyMarsh
Copy link
Contributor

@TinyMarsh TinyMarsh commented Jul 3, 2024

This PR adds much of the functionality to calculate the stats specified in channels_. A lot of the logic for this is copied from the existing code, but modified to use the calculated_stats_ vector in place of the existing series object. As a reminder, this is necessary because the calculated stats are being stored in a flat vector (calculated_stats_) which represents a multidimensional array of factors that the user wants to group by, as opposed to the series object. A result of this is the introduction of the calculate_index method, and the get_channel_index method - both necessary to find the correct index of the factor bin, and "channel" stat in the calculated_stats_ vector, respectively.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there, I think. I do think it would be worth adding a test for the calculate_index function though and I've made a few other small suggestions.

The Windows CI runner is failing because of some type narrowing warning again 😞

src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Show resolved Hide resolved
Comment on lines 48 to 49
std::vector<std::string> channels_;
std::unordered_map<std::string, int> channel_index_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might not be completely understanding what these data structures do... But am I right in thinking channel_index_ contains indexes into channels_? If so, could you just make channels_ a std::map<std::string, std::string> (I'm guessing it needs to be ordered...) and drop channels_index_?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/HealthGPS/analysis_module.cpp Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Show resolved Hide resolved
src/HealthGPS/analysis_module.cpp Outdated Show resolved Hide resolved
@TinyMarsh TinyMarsh requested a review from alexdewar July 17, 2024 23:34
Copy link
Contributor

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I'd probs have a go at #470 before merging this, just so we can be confident that the indexing works, but it's up to you. I don't think it needs lots of test cases -- just a few basic ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants